Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: the flake8 max line failure E501 and some other errors #2133

Closed
wants to merge 1 commit into from
Closed

fix: the flake8 max line failure E501 and some other errors #2133

wants to merge 1 commit into from

Conversation

rohit2p
Copy link
Contributor

@rohit2p rohit2p commented Jan 6, 2024

Purpose of PR?:

Fixes #2111
If the changes in this PR are manually verified, list down the scenarios covered::

Before
Untitled

After
Untitled

Checklist:

This pull request addresses several flake8 errors and undefined name issues in the codebase. I have adjusted the indentation, comments, and variable definitions to comply with the flake8 style guide. Additionally, i removed the max-line-length = 127 from the python-flake8 because it was overwriting setup.cfg. These changes ensure that the codebase adheres to the project's coding standards and resolves the reported issues.

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please send the pull request to develop.

and we don't test docs for flake8 because this is automated created, we can't change that.

@rohit2p rohit2p changed the base branch from stable to develop January 6, 2024 15:44
@@ -31,4 +31,4 @@ jobs:
run: |
python -m pip install --upgrade pip
pip install flake8
flake8 --count --max-line-length=127 --statistics mslib tests
flake8 --count --statistics mslib tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't run it on docs, see parameters.

Copy link
Member

@ReimarBauer ReimarBauer Jan 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you fixed docs now, then extend the parameters.

But I am not sure if that makes sense, when make docs is run, then other py scripts will become created below docs, which are automated done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do i do now then?

Copy link
Member

@ReimarBauer ReimarBauer Jan 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you did a bit too much, as in the "good first" issue was defined, read line 34, extend it to the docs dir too.

But when you do this the error will block its merging. so it must be solved too. And that gets then easily into not been a good first issue.

So leave it as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok understood. actually its my first tym doing a contribution that way i got confused.
so here is the thing i think i should do now say yes and if i understood the assignment correctly.
i will fix all the issue in #2111 but this time from the stable branch.
and i will leave the python-flake8 file as it is
?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't touch the conf.py - no changes. Only remove the extra parameter from the CI file.

Look also in my comment in the slack channel

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason for good first issue, is to get a simple thing done, by learning the workflow. There bigger other issues later.

@rohit2p
Copy link
Contributor Author

rohit2p commented Jan 6, 2024

at this page https://open-mss.github.io/develop/GSOC/ideas i read that all bug fix go to stable branch that's why i did that😔

@ReimarBauer
Copy link
Member

at this page https://open-mss.github.io/develop/GSOC/ideas i read that all bug fix go to stable branch that's why i did that😔

obviously you have not created a branch from stable but from develop.

When you want to send it to stable, use the right branch from the beginning ;)

A fix should not get all changes of develop into stable ;)

@ReimarBauer
Copy link
Member

Do you want to redo for stable?

@rohit2p
Copy link
Contributor Author

rohit2p commented Jan 6, 2024

yes i can do that
but i have a question ?
like when i do that should i leave this part as it is

      run: |
        python -m pip install --upgrade pip
        pip install flake8
        flake8 --count --max-line-length=127 --statistics mslib tests

and only make changes in the work script ??
i dont't get it

@ReimarBauer
Copy link
Member

i dont't get it

only this

only_this

@rohit2p rohit2p closed this Jan 6, 2024
@rohit2p rohit2p deleted the fix/flake8-max-line-failure branch January 6, 2024 16:32
@ReimarBauer
Copy link
Member

Now we have 2 PR for this?

@rohit2p
Copy link
Contributor Author

rohit2p commented Jan 9, 2024

yea but i closed it since in this the branch was created from develop and i did a bit too much so i redid from stable

@ReimarBauer
Copy link
Member

Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

python-flake8 max-line-length failure
2 participants